Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf: store code in Bytes #13

Closed
wants to merge 1 commit into from
Closed

perf: store code in Bytes #13

wants to merge 1 commit into from
+9 −20

Conversation

mooori
Copy link

@mooori mooori commented Jul 14, 2022

In #448 it is pointed out that storing code in Bytes rather than Rc<Vec<u8>> avoids an extra indirection in opcode fetch. This change is implemented in this PR.

Public interfaces

Machine::new() and Runtime::new() are changed to have parameter code: Bytes instead of code: Rc<Vec<u8>>. Without this change, NEAR gas usage increases as constructing Bytes then would require cloning the Vec inside the Rc. (See numbers below.)

While this doesn't break aurora-engine, it might break other things. For example, jsontests would require an update to run on this fork.

NEAR gas usage

Including this change in aurora-engine leads to test failures due to NEAR gas usage which is lower than expected:

---- tests::repro::repro_8ru7VEA stdout ----
thread 'tests::repro::repro_8ru7VEA' panicked at 'assertion failed: `(left == right)`
  left: `242`,
 right: `241`', engine-tests/src/tests/repro.rs:146:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::repro::repro_D98vwmi stdout ----
thread 'tests::repro::repro_D98vwmi' panicked at 'assertion failed: `(left == right)`
  left: `199`,
 right: `198`', engine-tests/src/tests/repro.rs:146:5

---- tests::repro::repro_5bEgfRQ stdout ----
thread 'tests::repro::repro_5bEgfRQ' panicked at 'assertion failed: `(left == right)`
  left: `706`,
 right: `705`', engine-tests/src/tests/repro.rs:146:5

---- tests::repro::repro_GdASJ3KESs stdout ----
thread 'tests::repro::repro_GdASJ3KESs' panicked at 'assertion failed: `(left == right)`
  left: `133`,
 right: `132`', engine-tests/src/tests/repro.rs:146:5

---- tests::uniswap::test_uniswap_input_multihop stdout ----
thread 'tests::uniswap::test_uniswap_input_multihop' panicked at 'assertion failed: `(left == right)`
  left: `123`,
 right: `122`', engine-tests/src/tests/uniswap.rs:41:5
NEAR gas usage if public interface are not changed and instead `Vec` is cloned: 
---- tests::repro::repro_8ru7VEA stdout ----
thread 'tests::repro::repro_8ru7VEA' panicked at 'assertion failed: `(left == right)`
  left: `242`,
 right: `247`', engine-tests/src/tests/repro.rs:146:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::repro::repro_D98vwmi stdout ----
thread 'tests::repro::repro_D98vwmi' panicked at 'assertion failed: `(left == right)`
  left: `199`,
 right: `205`', engine-tests/src/tests/repro.rs:146:5

---- tests::repro::repro_5bEgfRQ stdout ----
thread 'tests::repro::repro_5bEgfRQ' panicked at 'assertion failed: `(left == right)`
  left: `706`,
 right: `705`', engine-tests/src/tests/repro.rs:146:5

---- tests::repro::repro_FRcorNv stdout ----
thread 'tests::repro::repro_FRcorNv' panicked at 'assertion failed: `(left == right)`
  left: `197`,
 right: `202`', engine-tests/src/tests/repro.rs:146:5

---- tests::repro::repro_GdASJ3KESs stdout ----
thread 'tests::repro::repro_GdASJ3KESs' panicked at 'assertion failed: `(left == right)`
  left: `133`,
 right: `136`', engine-tests/src/tests/repro.rs:146:5

---- tests::uniswap::test_uniswap_exact_output stdout ----
thread 'tests::uniswap::test_uniswap_exact_output' panicked at '34 Tgas is not equal to 33 Tgas', engine-tests/src/test_utils/mod.rs:842:5

---- tests::uniswap::test_uniswap_input_multihop stdout ----
thread 'tests::uniswap::test_uniswap_input_multihop' panicked at 'assertion failed: `(left == right)`
  left: `123`,
 right: `125`', engine-tests/src/tests/uniswap.rs:41:5

---- tests::one_inch::test_1inch_liquidity_protocol stdout ----
thread 'tests::one_inch::test_1inch_liquidity_protocol' panicked at '26 Tgas is not equal to 25 Tgas', engine-tests/src/test_utils/mod.rs:842:5

Question

Does this change make sense given that it requires breaking public interfaces for rather small improvements in NEAR gas usage?

@mfornet mfornet requested review from joshuajbouw and birchmd July 18, 2022 11:18
@birchmd
Copy link
Member

birchmd commented Jul 18, 2022

Thanks for looking into this @mooori . In my opinion, it is not worth making a breaking change for a less than 1% savings.

@mooori mooori closed this Jul 27, 2022
aleksuss added a commit to aleksuss/sputnikvm that referenced this pull request Jul 3, 2024
@aleksuss
test_32 bit aurora-is-near#13
d94368c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuajbouw joshuajbouw Awaiting requested review from joshuajbouw

@birchmd birchmd Awaiting requested review from birchmd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mooori @birchmd